feat: drop support for PHP < 8.1, symfony < 6.4 (WIP)#368
feat: drop support for PHP < 8.1, symfony < 6.4 (WIP)#368Chris53897 wants to merge 12 commits intogeocoder-php:masterfrom
Conversation
|
Should we align with Symfony 6.4 php >=8.1 requirement? |
|
I can do if you want. EOL is in 2 months https://www.php.net/supported-versions.php |
|
Well Symfony 6.4 won't drop support for 8.1, so i think it's better, unless 8.2 has something you really want to use :) |
|
I changed it to PHP 8.1
|
|
Well yeah, doctrine bundle v3 support could be nice, but then we should add lowest/higest deps to the matrix |
Chris53897
changed the title
WalkthroughThe PR upgrades the bundle to PHP 8.1+ and Symfony 6.4/7.x support while removing compatibility with older versions. Version-specific test configurations are consolidated, trait-based helpers are simplified, CI workflows are updated with newer PHP versions and actions, and deprecated providers are removed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/Functional/PluginInteractionTest.php (1)
45-46: Unconditional skip hides a real regression in plugin/cache interaction
markTestSkipped()at the top means this test will never execute its assertions, so any regressions in the cache + FakeIp + geoPlugin interaction will go unnoticed. The TODO shows this is temporary, but before merging a “drop old versions” PR intomaster, it would be better to either:
- Fix the underlying “serialization of closure” issue, or
- Make the skip clearly traceable (e.g. link to an issue / add
@group/ short explanation of when this fails), or- Gate the skip on a known condition (PHP / Symfony / library version) instead of skipping unconditionally.
If you share the exact stack trace for the serialization error, I can help sketch a concrete fix that avoids needing to skip the test.
CHANGELOG.md (1)
5-15: Changelog 6.0 entry should also mention dropped Symfony versionsYou document the PHP baseline change and factory removals, but the PR also drops Symfony < 6.4 support. Consider adding a “Removed: Symfony < 6.4 support” bullet under Version 6.0 so consumers see the full BC surface.
tests/Functional/config/framework.yml (1)
11-14: Verify framework annotations/error-handling choices for functional testsThese additions change global framework behavior:
annotations: falsedisables FrameworkBundle’s annotation support; double‑check that your functional tests (routing, validation, etc.) don’t depend on annotations anywhere.handle_all_throwables: trueandphp_errors.log: trueaffect how exceptions and PHP errors are reported and logged in tests.Given the concurrent PHPUnit config changes and the commented-out
ErrorHandler::register()in tests/bootstrap.php, it’s worth confirming that this combination still surfaces the errors and deprecations you care about rather than silently logging them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
.github/workflows/ci.yml(5 hunks)CHANGELOG.md(1 hunks)composer.json(1 hunks)doc/services.md(0 hunks)phpunit.xml.dist(2 hunks)tests/Functional/BundleInitializationTest.php(0 hunks)tests/Functional/CustomTestKernel.php(2 hunks)tests/Functional/GeocoderListenerTest.php(0 hunks)tests/Functional/Helper/CacheHelper.php(1 hunks)tests/Functional/Helper/CacheHelperV7.php(0 hunks)tests/Functional/Helper/CacheHelperV8.php(0 hunks)tests/Functional/PluginInteractionTest.php(1 hunks)tests/Functional/ProviderFactoryTest.php(0 hunks)tests/Functional/config/framework.yml(1 hunks)tests/Functional/config/framework_sf6.yml(0 hunks)tests/Functional/config/listener.yml(1 hunks)tests/Functional/config/listener_php7.yml(0 hunks)tests/Functional/config/listener_php8.yml(0 hunks)tests/Functional/config/provider/geoips.yml(0 hunks)tests/Functional/config/provider/mapzen.yml(0 hunks)tests/bootstrap.php(1 hunks)
💤 Files with no reviewable changes (11)
- tests/Functional/config/framework_sf6.yml
- tests/Functional/config/listener_php8.yml
- doc/services.md
- tests/Functional/Helper/CacheHelperV7.php
- tests/Functional/GeocoderListenerTest.php
- tests/Functional/config/provider/geoips.yml
- tests/Functional/config/provider/mapzen.yml
- tests/Functional/Helper/CacheHelperV8.php
- tests/Functional/ProviderFactoryTest.php
- tests/Functional/BundleInitializationTest.php
- tests/Functional/config/listener_php7.yml
🧰 Additional context used
🪛 GitHub Actions: CI
tests/Functional/PluginInteractionTest.php
[warning] 45-45: Test skipped: TODO solve serialization of closure error
composer.json
[error] 1-1: Composer install failed due to PHP version mismatch: willdurand/geocoder requires PHP >= 8.2, but current PHP is 8.1.33. This blocks dependency resolution (geocoder-php/yandex-provider also requires willdurand/geocoder ^4.0|^5.0). Consider upgrading PHP or adjusting dependencies. Command: bin/composer_install.sh
🪛 PHPMD (2.15.0)
tests/Functional/Helper/CacheHelper.php
24-24: Avoid unused parameters such as '$key'. (undefined)
(UnusedFormalParameter)
24-24: Avoid unused parameters such as '$default'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$key'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$value'. (undefined)
(UnusedFormalParameter)
28-28: Avoid unused parameters such as '$ttl'. (undefined)
(UnusedFormalParameter)
33-33: Avoid unused parameters such as '$key'. (undefined)
(UnusedFormalParameter)
43-43: Avoid unused parameters such as '$keys'. (undefined)
(UnusedFormalParameter)
43-43: Avoid unused parameters such as '$default'. (undefined)
(UnusedFormalParameter)
48-48: Avoid unused parameters such as '$values'. (undefined)
(UnusedFormalParameter)
48-48: Avoid unused parameters such as '$ttl'. (undefined)
(UnusedFormalParameter)
53-53: Avoid unused parameters such as '$keys'. (undefined)
(UnusedFormalParameter)
58-58: Avoid unused parameters such as '$key'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (11)
tests/Functional/Helper/CacheHelper.php (2)
17-22: LGTM: Consolidation simplifies the codebase.Replacing version-specific traits with a unified class is appropriate given the PHP 8.1+ requirement. This reduces complexity and eliminates the need to maintain separate implementations.
28-61: Static analysis warnings are expected for stub implementations.PHPMD flags unused parameters throughout these methods. This is expected and acceptable for a test helper with no-op stub implementations. The methods appropriately return success values or empty results for cache operations that don't need to perform actual caching in tests.
tests/Functional/config/listener.yml (3)
16-22: Verify the attribute mapping configuration and fixture entities are properly migrated.The mapping has been converted to use attribute-based configuration (line 19:
type: attribute) to align with PHP 8 native attributes rather than doctrine/annotations. Ensure that all entity classes intests/Functional/Fixtures/Entityhave been updated to use PHP attributes instead of annotations. Check that entity files use#[ORM\...]syntax and no longer contain legacy@ORM\...docblock annotations.
28-33: The root_url and user_agent options are supported by NominatimFactory.Both
root_urlanduser_agentare documented configuration options for the Nominatim provider. Theuser_agentparameter is required by Nominatim's usage policy, androot_urldefaults tohttps://nominatim.openstreetmap.org. The configuration in this file correctly uses these options.
40-46: Verify GeocoderListener service definition arguments and event listener tag.The
doctrine.event_listenertag withonFlushevent is a valid Symfony/Doctrine configuration pattern. Service references using fully-qualified class names (FQCN) like@Bazinga\GeocoderBundle\Mapping\Driver\AttributeDriverare standard Symfony autowiring syntax and will be correctly resolved.However, verify that
Bazinga\GeocoderBundle\Doctrine\ORM\GeocoderListenerconstructor accepts exactly these two arguments in this order:
- The acme provider service
- The AttributeDriver service
Check the actual
GeocoderListenerclass implementation to confirm the constructor signature matches the argument order shown in the service definition.tests/Functional/CustomTestKernel.php (2)
64-77:kernel.runtime_mode.webparameter addition looks appropriateExposing
kernel.runtime_mode.webastruein the kernel parameters aligns with the newer Symfony runtime expectations and should help tests behave like a web runtime without affecting older Symfony versions.
33-38: Confirmreboot()signature compatibility withTestKernelAdding the
: voidreturn type is good, but this override still takes a nullable$warmupDirwithout a default. If the parentTestKernel::reboot()has$warmupDir = null, calling$kernel->reboot()with no argument on aCustomTestKernelinstance would be a signature mismatch at runtime. Verify the parent signature and either mirror its default or ensure you never callreboot()without an explicit argument.phpunit.xml.dist (1)
8-10: Disabling PHPUnit error/notice/warning to exception conversion weakens test signalWith
convertErrorsToExceptions,convertNoticesToExceptions, andconvertWarningsToExceptionsall set tofalse, PHP issues may no longer fail tests—especially sinceErrorHandler::register()is currently commented out in tests/bootstrap.php. Before merging, please either restore these totrueor have a clear alternative error-handling path so failures don't slip through unnoticed.tests/bootstrap.php (1)
5-8: Commented-outErrorHandler::register()leaves tests without robust error handlingThe
use ErrorHandlerandErrorHandler::register(null, false);lines are currently commented out, so Symfony's ErrorHandler is never registered during tests. Combined with the disabledconvert*ToExceptionsflags in phpunit.xml, this risks turning many PHP errors/notices/warnings into mere log entries instead of test failures.Once you've resolved the PHPUnit / errorLogger / deprecation handling issues, please either:
- Re‑enable
ErrorHandler::register()(and potentially restore the default PHPUnit conversion flags), or- Document and implement a different, explicit strategy for surfacing PHP issues in tests.
.github/workflows/ci.yml (1)
65-70: Align PHPUnit PHP matrix with composer/geocoder constraints and consider lowest/highest runsThe PHPUnit matrix still runs on PHP 8.1, but with
willdurand/geocoderconstrained to^5.0the dependency resolution on 8.1 currently fails (as seen in the pipeline). Once you decide whether to keep 8.1 support or move to 8.2+ incomposer.json, please update this matrix accordingly so that all matrix entries are actually installable.Also, given the discussion about
doctrine/doctrine-bundlev2 vs v3 and the many upgraded provider packages, it may be worth adding dedicated "lowest" and "highest" dependency runs (e.g., usingramsey/composer-install'sdependency-versionsinput) so that both extremes are covered in CI.composer.json (1)
17-25: Fix PHP 8.1 vswilldurand/geocoder>= 8.2 conflict and tidy composer metadataThe current constraints are inconsistent with each other and with the CI matrix:
phpis declared as^8.1, and CI runs PHPUnit on PHP 8.1.willdurand/geocoderis constrained to^5.0, but the version pulled in requires PHP >= 8.2, which is exactly what the pipeline failure reports.- Result: the PHP 8.1 job cannot resolve dependencies.
Before merging, you'll need to choose one of these directions:
Keep PHP 8.1 support
- Pin
willdurand/geocoderto the latest release that still supports PHP 8.1 (or keep the^4.xrange if needed), and ensure all geocoder-* provider versions remain compatible.Drop PHP 8.1 and require 8.2+
- Bump
"php"to^8.2(and adjust CI and CHANGELOG wording accordingly).- Remove PHP 8.1 from the test matrix.
Additional small cleanups while you're here:
symfony/options-resolvercurrently has a leading space in the constraint:" ^6.4 || ^7.0". Consider normalizing it to"^6.4 || ^7.0"for consistency.doctrine/doctrine-bundleis fixed at"^2.18". Given the PR discussion about supporting v3, you may want to widen this to"^2.18 || ^3.0"and cover both in CI.- The branch alias still reads
"dev-master": "5.0-dev"even though you're introducing a 6.0 changelog entry; consider bumping this to"6.0-dev"when you're ready to tag v6.
| public function get(string $key, mixed $default = null): mixed | ||
| { | ||
| } |
There was a problem hiding this comment.
Return the default parameter instead of implicit null.
The get() method has an empty body, which implicitly returns null. Per the PSR-16 CacheInterface contract, it should return the $default parameter when the key is not found. Tests that pass custom default values may receive null instead of their expected default.
Apply this diff:
public function get(string $key, mixed $default = null): mixed
{
+ return $default;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function get(string $key, mixed $default = null): mixed | |
| { | |
| } | |
| public function get(string $key, mixed $default = null): mixed | |
| { | |
| return $default; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
24-24: Avoid unused parameters such as '$key'. (undefined)
(UnusedFormalParameter)
24-24: Avoid unused parameters such as '$default'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In tests/Functional/Helper/CacheHelper.php around lines 24 to 26, the get(string
$key, mixed $default = null): mixed method is empty and therefore returns null;
implement it to follow PSR-16 by returning the provided $default when the key is
not found. Update the method to look up the key in the underlying store (or
check whatever internal array/registry this helper uses) and return the stored
value if present, otherwise return $default, preserving the method signature and
not throwing for missing keys.
|
@Chris53897 i think it's better to do smaller PR's. I've started with removing deprecated provider factories. |
|
I agree. |
|
Seems it's not so easy to go small either 😅 |
|
@Chris53897 ok it's ready now to make small prs |
|
So i should make new PRs from main-Branch? Or work on this PR? |
|
|
||
| // use Symfony\Component\ErrorHandler\ErrorHandler; | ||
|
|
||
| // ErrorHandler::register(null, false); |
There was a problem hiding this comment.
I am not sure. Higher PHPUnit Versions than 9 often complains about "tests are not removing there own error handler"
There was a problem hiding this comment.
ah yes, but i think thats applicable only to phpunit v10
| report_fields_where_declared: true | ||
| naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware | ||
| auto_mapping: false | ||
| dbal: |
There was a problem hiding this comment.
maybe restore indentation for less diff?
do how it's easier for you |
|
ok. i will try to find some time tomorrow or on the weekend to work on this. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
composer.json (1)
27-29: Clarify whether doctrine/doctrine-bundle v3 support is intended for this PR.doctrine/doctrine-bundle v3.0+ requires Symfony ^6.4 || ^7.0 || ^8.0 and PHP ^8.4, which aligns with your project targets. However, the current constraint pins to
^2.18only (line 28). If v3 support is intended, update the constraint to^2.18 || ^3.0. Additionally, note that bundle v3 strictly requires doctrine/orm ^3.0, so if bundle v3 is added, verify the orm constraint^2.8 || ^3.0doesn't create a conflict for bundle v3 deployments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
composer.json(1 hunks)tests/Functional/BundleInitializationTest.php(0 hunks)tests/Functional/ProviderFactoryTest.php(0 hunks)
💤 Files with no reviewable changes (2)
- tests/Functional/BundleInitializationTest.php
- tests/Functional/ProviderFactoryTest.php
🔇 Additional comments (1)
composer.json (1)
18-23: Thephp-http/discoveryplugin being disabled is intentional and correct.The code uses runtime discovery via
Psr18ClientDiscovery::find()(not plugin-based discovery), which works regardless of theallow-pluginssetting. Every factory supports an optionalhttp_clientconfiguration parameter; if not provided, runtime discovery automatically detects any installed PSR-18 compatible client. No change needed.
| "symfony/console": "^6.4 || ^7.0", | ||
| "symfony/framework-bundle": "^6.4 || ^7.0", | ||
| "symfony/options-resolver": " ^6.4 || ^7.0", | ||
| "willdurand/geocoder": "^4.6.0 || ^5.0" |
There was a problem hiding this comment.
Fix the invalid/odd constraint formatting on symfony/options-resolver.
Line 23 has a leading space (" ^6.4 || ^7.0"), which is easy to miss and can cause constraint parsing/lockfile churn—normalize it to match the other Symfony constraints.
- "symfony/options-resolver": " ^6.4 || ^7.0",
+ "symfony/options-resolver": "^6.4 || ^7.0",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "symfony/console": "^6.4 || ^7.0", | |
| "symfony/framework-bundle": "^6.4 || ^7.0", | |
| "symfony/options-resolver": " ^6.4 || ^7.0", | |
| "willdurand/geocoder": "^4.6.0 || ^5.0" | |
| "symfony/console": "^6.4 || ^7.0", | |
| "symfony/framework-bundle": "^6.4 || ^7.0", | |
| "symfony/options-resolver": "^6.4 || ^7.0", | |
| "willdurand/geocoder": "^4.6.0 || ^5.0" |
🤖 Prompt for AI Agents
In composer.json around lines 21 to 24, the version constraint for
"symfony/options-resolver" has a leading space (" ^6.4 || ^7.0") which is
inconsistent with the other Symfony dependencies and can break constraint
parsing; remove the leading space so the constraint reads like the others (e.g.
"^6.4 || ^7.0") to normalize formatting and prevent lockfile churn.
|
I made new smaller PR. |
3 participants
TODO:
Summary by CodeRabbit
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.